Skip to content

fix: normalize PersistenceLength by n_frames instead of trajectory n_frames - #5456

Open
shubhrai23 wants to merge 2 commits into
MDAnalysis:developfrom
shubhrai23:fix/issue-5453-persistencelength-frames
Open

fix: normalize PersistenceLength by n_frames instead of trajectory n_frames#5456
shubhrai23 wants to merge 2 commits into
MDAnalysis:developfrom
shubhrai23:fix/issue-5453-persistencelength-frames

Conversation

@shubhrai23

@shubhrai23 shubhrai23 commented Aug 11, 2026

Copy link
Copy Markdown

Resolves #5453

Copilot AI lite review requested due to automatic review settings August 11, 2026 22:14
Copilot AI reviewed Aug 11, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes incorrect normalization in PersistenceLength when run(start=..., stop=...) processes only a subset of trajectory frames, ensuring results are normalized by the number of analyzed frames rather than the total trajectory length.

Changes:

  • Normalize results.bond_autocorrelation using self.n_frames (frames actually processed by the analysis run) instead of self._trajectory.n_frames (total frames in the trajectory).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@read-the-docs-community

read-the-docs-community Bot commented Aug 11, 2026 •
edited
Loading

Copy link
Copy Markdown

Documentation build overview

📚 MDAnalysis | 🛠️ Build #34075537 | 📁 Comparing 720cf66 against latest (c882e5c)

  🔍 Preview build  

3 files changed
± index.html
± documentation_pages/analysis/wbridge_analysis.html
± _modules/MDAnalysis/analysis/polymer.html

@codecov

codecov Bot commented Aug 11, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.87%. Comparing base (c882e5c) to head (720cf66).
⚠️ Report is 3 commits behind head on develop.

Additional details and impacted files 
@@           Coverage Diff            @@
##           develop    #5456   +/-   ##
========================================
  Coverage    93.87%   93.87%           
========================================
  Files          182      182           
  Lines        22522    22522           
  Branches      3206     3206           
========================================
  Hits         21143    21143           
  Misses         917      917           
  Partials       462      462

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

BradyAJohnston
BradyAJohnston requested changes Aug 16, 2026
View reviewed changes

@BradyAJohnston BradyAJohnston left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot review anything until you have properly used the PR template and properly filled it. Please do note remove the template when opening a PR.

@orbeckst orbeckst added more information needed Please reply to requests for information or the issue will be closed. close? Evaluate if issue/PR is stale and can be closed. labels Sep 2, 2026
@orbeckst

orbeckst commented Sep 2, 2026

Copy link
Copy Markdown
Member

@shubhrai23 please restore the PR template https://github.com/MDAnalysis/mdanalysis/blob/develop/.github/PULL_REQUEST_TEMPLATE.md and fill in all the details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Lite
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@BradyAJohnston BradyAJohnston BradyAJohnston requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

close? Evaluate if issue/PR is stale and can be closed. more information needed Please reply to requests for information or the issue will be closed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

PersistenceLength.run(start=, stop=) rescales entire results.bond_autocorrelation array by n_frames_processed / n_frames_total_trajectory

4 participants

@shubhrai23 @orbeckst @BradyAJohnston